Fix: Helm post-delete hook#7678
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Helm chart's uninstallation process where the post-delete hook was inadvertently removing cluster-scoped namespaces. By modifying the cleanup script to filter out specific configuration files, the change ensures that essential system namespaces remain intact on the host cluster while maintaining proper resource cleanup for other components. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Welcome @Varshar21-07! It looks like this is your first PR to karmada-io/karmada 🎉 |
karmada-bot
added
the
size/XS
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the post-delete job template in the Karmada Helm chart to skip deleting the system-namespace.yaml file from /opt/static-resources/ during cleanup. The review feedback suggests optimizing the bash loop by using built-in parameter expansion (${file##*/}) instead of spawning a subshell with basename, and adding an existence check [[ -e "$file" ]] to prevent errors if the glob pattern matches nothing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for file in /opt/static-resources/*; do | ||
| if [ "$(basename "$file")" != "system-namespace.yaml" ]; then | ||
| kubectl delete -f "$file" --ignore-not-found=true -R || true | ||
| fi | ||
| done |
There was a problem hiding this comment.
Using $(basename "$file") inside a loop spawns a subshell and executes an external binary (basename) for every file in the directory. Since the script is executed using bash, we can use bash's built-in parameter expansion ${file##*/} instead. This is more efficient, avoids spawning external processes, and is more robust as it doesn't rely on the availability of the basename command in the container image.
Additionally, we can add a check [[ -e "$file" ]] to ensure the file exists before attempting to delete it, preventing errors if the glob pattern does not match any files.
for file in /opt/static-resources/*; do
if [[ -e "$file" && "${file##*/}" != "system-namespace.yaml" ]]; then
kubectl delete -f "$file" --ignore-not-found=true -R || true
fi
doneThere was a problem hiding this comment.
Pull request overview
This PR fixes a Helm post-delete hook behavior in the Karmada chart that could delete cluster-scoped Namespace objects on the host cluster during helm uninstall, by excluding system-namespace.yaml from the host-side static resource cleanup.
Changes:
- Replaced
kubectl delete -f /opt/static-resources/ ... -Rwith a loop that deletes each static resource file individually. - Explicitly skips
system-namespace.yamlso host cluster namespaces likekarmada-system/karmada-clusterare not deleted during uninstall.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7678 +/- ##
==========================================
- Coverage 42.06% 42.06% -0.01%
==========================================
Files 879 879
Lines 54831 54831
==========================================
- Hits 23063 23062 -1
Misses 30023 30023
- Partials 1745 1746 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Varsha <varsha@Varshas-MacBook-Pro.local>
Varshar21-07
force-pushed
the
issue-7618-helm-post-delete
branch
from
31fee00 to
015e8f8
Compare
Signed-off-by: Varsha <varsha@Varshas-MacBook-Pro.local>
|
Hi @RainbowMango @XiShanYongYe-Chang @jrkeen, I've submitted this fix. Please review this and let me know if any changes needed. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR resolves a critical issue in the Helm chart where uninstalling the chart via the post-delete hook accidentally deletes cluster-scoped Namespace objects (like karmada-system and karmada-cluster) on the host cluster.
During uninstallation, the post-delete-job.yaml hook ran kubectl delete -f /opt/static-resources/ against the host cluster. Because system-namespace.yaml is located in that directory, the host cluster was instructed to delete those namespaces, cascading and destroying all resources inside them.
This PR replaces the broad kubectl delete -f command with a shell loop that explicitly ignores system-namespace.yaml during the cleanup process, preserving the namespaces on the host cluster while maintaining the cleanup logic for all other static resources.
Which issue(s) this PR fixes:
Fixes #7618
Special notes for your reviewer:
This has been manually verified by installing the Helm chart with --set systemNamespace=karmada-test-system onto a host cluster, and confirming that karmada-test-system is no longer deleted from the host cluster upon running helm uninstall.
Does this PR introduce a user-facing change?: